Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't expand unused vars #79

Merged
merged 2 commits into from
Jun 9, 2021
Merged

Don't expand unused vars #79

merged 2 commits into from
Jun 9, 2021

Conversation

jwodder
Copy link
Member

@jwodder jwodder commented Jun 8, 2021

Fixes #78.

@jwodder jwodder added the patch Increment the patch version when merged label Jun 8, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2021

Codecov Report

Merging #79 (8ae7081) into master (53752f3) will increase coverage by 62.77%.
The diff coverage is 77.77%.

❗ Current head 8ae7081 differs from pull request most recent head cd1fe71. Consider uploading reports for the commit cd1fe71 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master      #79       +/-   ##
===========================================
+ Coverage    3.80%   66.58%   +62.77%     
===========================================
  Files           8        9        +1     
  Lines         735      790       +55     
  Branches      107      115        +8     
===========================================
+ Hits           28      526      +498     
+ Misses        707      211      -496     
- Partials        0       53       +53     
Impacted Files Coverage Δ
src/tinuous/github.py 49.50% <ø> (+49.50%) ⬆️
src/tinuous/util.py 68.81% <74.54%> (+22.15%) ⬆️
src/tinuous/appveyor.py 79.26% <100.00%> (+79.26%) ⬆️
src/tinuous/travis.py 65.74% <100.00%> (+65.74%) ⬆️
src/tinuous/_version.py 100.00% <0.00%> (ø)
src/tinuous/__main__.py 48.46% <0.00%> (+48.46%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61dae01...cd1fe71. Read the comment docs.

elif key in kwargs:
return kwargs[key]
elif key in self.expanded_vars:
return self.expanded_vars[key]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh -- that seems like quite an elaborate (smells like a more proper of cause) solution! ;) Since would be in the "core" of things, could you add unittests for not yet covered cases in this function?

else:
assert index is not None # type: ignore[unreachable]
try:
sl = parse_slice(index)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided not to push on subclassing string.Formatter whenever we were talking about enhancing the extra formatters (like for shortened commit) since iirc it was lacking slicing, so here you are (re)implementing it?
Please add tests to cover except clause here and common cases like [-3:] and [start:end].

On a positive side -- this would open the possibility for custom formatter directives (in addition to r) ;-) ! (I am still wondering about git describe for commits ;))

@jwodder
Copy link
Member Author

jwodder commented Jun 8, 2021

@yarikoptic Further tests added.

@@ -8,6 +8,7 @@ minversion = 3.3.0
deps =
pytest~=6.0
pytest-cov~=2.0
pytest-mock~=3.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to myself: apparently we do not have tests extra_requires and all test requirements are set in tox making it the only kosher way to run tests.

@yarikoptic
Copy link
Member

ok, let's proceed with this one, thank you @jwodder !

@yarikoptic yarikoptic merged commit 3df2573 into master Jun 9, 2021
@yarikoptic yarikoptic deleted the gh-78 branch June 9, 2021 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fetching releases failed
3 participants